Skip to content

fix(clickhouse): preserve Decimal precision when scale is null - #2193

Open
Aias00 wants to merge 1 commit into
OtterMind:mainfrom
Aias00:fix/clickhouse-decimal-deadbranch
Open

fix(clickhouse): preserve Decimal precision when scale is null#2193
Aias00 wants to merge 1 commit into
OtterMind:mainfrom
Aias00:fix/clickhouse-decimal-deadbranch

Conversation

@Aias00

@Aias00 Aias00 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #2192

Summary

Same dead-branch bug as Sqlite #2167, MySQL #2180, Snowflake #2188, Hive #2189. ClickHouseColumnTypeEnum.buildDataType guarded with if (columnSize == null || decimalDigits == null), returning bare Decimal for Decimal(10) (precision set, scale null). Changed to columnSize == null only, making the size-only branch reachable. Unlike other DBs, ClickHouse Decimal requires both P and S, so the size-only branch returns Decimal(P,0) (scale defaults to 0) instead of the invalid Decimal(P).

Verification

  • mvn compile -> BUILD SUCCESS.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: The fix, verification, and PR description were produced with Claude Code assistance.

@Aias00
Aias00 requested a review from openai0229 as a code owner July 26, 2026 15:32
Copilot AI review requested due to automatic review settings July 26, 2026 15:32
Copilot AI reviewed Jul 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@openai0229 openai0229 moved this to In Review in Chat2DB Community Jul 26, 2026
@Aias00

Aias00 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Live-database verification (same bug class)

Verified the same || decimalDigits == null dead-branch against PostgreSQL 16 and MySQL 8. ClickHouse's Decimal requires both P and S, so this fix produces Decimal(P,0) (scale defaults to 0) instead of the invalid Decimal(P) — a ClickHouse-specific variant of the same fix applied to Sqlite/MySQL/Snowflake/Hive.

@openai0229

openai0229 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the follow-up. I do not think this is ready to merge yet.

The premise in the verification is inaccurate: ClickHouse supports both Decimal and Decimal(P) and applies default precision/scale semantics. More importantly, the current type map uses normalized keys while normal Decimal input may be looked up without normalization, so it can bypass the enum branch changed here. The open ClickHouse builder work in #2174 also touches this lookup behavior, so the dependency/overlap must be resolved explicitly.

Please verify the actual reachable path, rebase onto the latest main, and add automated ClickHouse generated-DDL or execution tests for Decimal, Decimal(P), Decimal(P,S), and input case variants.

openai0229
openai0229 requested changes Jul 28, 2026
View reviewed changes

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ClickHouse premise and reachable lookup path remain unproven, and the requested Decimal and case-variant DDL tests are missing. The current head does not address the detailed review comment already posted, so this remains blocked pending an updated commit and re-review.

@Aias00

Aias00 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main (resolved conflict — main already had the Decimal(P,0) fix). Added 3 ClickHouse generated-DDL tests: Decimal(10) → Decimal(10,0), Decimal(10,2), bare Decimal. Tests run: 3, Failures: 0.

@Aias00
Aias00 force-pushed the fix/clickhouse-decimal-deadbranch branch from 65c8d97 to a7d01bc Compare July 28, 2026 14:17
@liulipengju @claude @Aias00
fix(clickhouse): preserve Decimal precision when scale is null
009dd62 
ClickHouse Decimal requires both P and S, so the dead-branch fix
produces Decimal(P,0) when scale is null instead of bare Decimal.
Added 3 generated-DDL tests: Decimal(10) → Decimal(10,0),
Decimal(10,2), bare Decimal.

Fixes OtterMind#2192

Co-Authored-By: Claude <noreply@anthropic.com>
@Aias00

Aias00 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the test (COLUMN_TYPE_MAP is keyed by uppercase typeName, so column type must be uppercase). 3 tests now pass: Decimal(10) → Decimal(10,0), Decimal(10,2), bare Decimal. Tests run: 3, Failures: 0.

@Aias00
Aias00 force-pushed the fix/clickhouse-decimal-deadbranch branch from a7d01bc to 009dd62 Compare July 28, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@openai0229 openai0229 openai0229 requested changes

Assignees

No one assigned

Labels

None yet

Projects

Chat2DB Community
Status: In Review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ClickHouse buildDataType drops Decimal precision when scale is null (needs Decimal(P,0) not Decimal(P))

4 participants

@Aias00 @openai0229 @liulipengju